dbms/src/Storages/KVStore/tests/gtest_new_kvstore.cpp (1)

1268-1268: Wall-clock sleep adds mild flakiness.

sleep_for(2s) combined with safe_point_update_interval_seconds=1 is fine in practice, but makes this test timing-dependent and slow. If you ever want to make it deterministic, the cache uses steady_clock inside getGCSafepointIfValid, so a custom clock injection or exposing an "expire now" hook would eliminate the sleep. Not required for this PR.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@dbms/src/Storages/KVStore/tests/gtest_new_kvstore.cpp` at line 1268, The test
uses a wall-clock sleep (std::this_thread::sleep_for(std::chrono::seconds(2)))
which makes it timing-dependent and slow; replace this with a deterministic
approach by injecting a test clock or adding an "expire now" hook so the
cache/GC safepoint logic can be advanced without sleeping — target the code
paths around getGCSafepointIfValid and the safe_point_update_interval_seconds
behavior (use a steady_clock-injectable implementation or call an exposed method
to force expiry) so the test can trigger the same cache refresh immediately and
remove the sleep.

dbms/src/Storages/KVStore/TiKVHelpers/PDTiKVClient.h (1)

198-225: Consider whether the CacheOnly path should emit tiflash_gc_safepoint_backoff_count{type=success}.

In the CacheOnly branch no PD request is ever made, so calling observe_backoff_count(true) (with backoff_count == 0) inflates the type_success histogram with samples that do not correspond to an actual PD fetch. Previously every observation in that metric reflected a real PD interaction; after this change the vast majority of observations will come from cache-only lookups on the hot read path and the metric will mostly report zeros. If the intent of the metric is to track PD-fetch backoff, consider skipping the observation on the CacheOnly cache-hit/miss path (and also skipping it on the existing fast-path cache hit).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@dbms/src/Storages/KVStore/TiKVHelpers/PDTiKVClient.h` around lines 198 - 225,
The CacheOnly branch and the fast-path cached-hit branch currently call
observe_backoff_count(true) even though no PD request occurs; remove those calls
so the tiflash_gc_safepoint_backoff_count{type=success} metric is only observed
when an actual PD fetch/backoff happens. Specifically, in PDTiKVClient.h inside
the code paths handling GCSafepointFetchStrategy::CacheOnly and the
getGCSafepointIfValid(cache-hit) branch, eliminate the
observe_backoff_count(true) calls and ensure observe_backoff_count is only
invoked in the code path(s) that perform a real PD fetch (the fallback PD-fetch
logic). Use the existing symbols ks_gc_sp_map.getGCSafepoint,
ks_gc_sp_map.getGCSafepointIfValid, and observe_backoff_count to locate and
adjust the calls.

dbms/src/Storages/StorageDeltaMerge.cpp (1)

915-915: checkStartTs is now invoked three times per read; with CacheOnly this is effectively identical to a single call.

Previously each checkStartTs call could trigger a PD fetch when the cache was stale, so invoking it pre-read / post-read / post-snapshot gave each call an independent chance to pick up a freshly advanced safepoint. With CacheOnly all three calls read the same cached value (unless a background path races in between), so the "ensure after read" invariant the comments describe no longer adds meaningful coverage. Worth a short note in the PR or code comments that the post-read checks are kept as a defense-in-depth against a future background refresh between the two calls, rather than active safety.

Also applies to: 1011-1011, 1057-1057

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@dbms/src/Storages/StorageDeltaMerge.cpp` at line 915, Multiple calls to
checkStartTs(mvcc_query_info.start_ts, context, query_info.req_id, keyspace_id)
now read the same cached safepoint under CacheOnly, so the post-read invocations
no longer increase coverage; update the code comment near the checkStartTs calls
(the pre-read/post-read/post-snapshot invocations) to state explicitly that with
CacheOnly the checks observe the same cached value and that the extra calls are
retained as defense-in-depth only (to catch a rare background refresh/race),
rather than for active additional safety, so future readers understand why we
keep the redundant calls.

dbms/src/Storages/KVStore/tests/gtest_new_kvstore.cpp (1)

1279-1279: Optional: reduce fixed wait time in expiry test.

2s works, but for a 1s validity window, a slightly-above-1s wait (e.g. 1100ms) keeps semantics and trims test runtime.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@dbms/src/Storages/KVStore/tests/gtest_new_kvstore.cpp` at line 1279, Replace
the long fixed 2s sleep in the expiry test's std::this_thread::sleep_for call
with a slightly-above-1s duration (e.g., 1100ms) so the test still exceeds the
1s validity window but runs faster; locate the std::this_thread::sleep_for(...)
invocation in the expiry test and change the duration to
std::chrono::milliseconds(1100).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

dbms/src/Storages/KVStore/tests/gtest_new_kvstore.cpp (1)

1226-1263: ⚠️ Potential issue | 🟡 Minor

Make cache cleanup assertion-safe with RAII.

If any ASSERT_* aborts early, the trailing cleanup won’t run and cached safepoint state can leak into later tests.

Proposed fix

 TEST(PDClientHelperTest, CacheOnlyReadPathDoesNotFetchFromPD)
 {
     constexpr KeyspaceID keyspace_id = 9527;
     PDClientHelper::removeKeyspaceGCSafepoint(keyspace_id);
+    SCOPE_EXIT({ PDClientHelper::removeKeyspaceGCSafepoint(keyspace_id); });

@@
-    PDClientHelper::removeKeyspaceGCSafepoint(keyspace_id);
 }

 TEST(PDClientHelperTest, CacheOnlyReadPathCanReturnExpiredCache)
 {
     constexpr KeyspaceID keyspace_id = 9528;
     PDClientHelper::removeKeyspaceGCSafepoint(keyspace_id);
+    SCOPE_EXIT({ PDClientHelper::removeKeyspaceGCSafepoint(keyspace_id); });

@@
-    PDClientHelper::removeKeyspaceGCSafepoint(keyspace_id);
 }

Based on learnings: Ensure `TearDown()` is called or cleanup helpers are used to avoid side effects on other tests.

Also applies to: 1268-1305

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@dbms/src/Storages/KVStore/tests/gtest_new_kvstore.cpp` around lines 1226 -
1263, Wrap the PDClientHelper::removeKeyspaceGCSafepoint(keyspace_id) cleanup in
an RAII/TearDown-safe mechanism so it always runs even if ASSERT_* aborts;
specifically, create a scoped cleanup (or move the test into a test fixture and
implement TearDown) that calls
PDClientHelper::removeKeyspaceGCSafepoint(keyspace_id) and use it around the
calls to PDClientHelper::getGCSafePointWithRetry and related assertions
(references: PDClientHelper::removeKeyspaceGCSafepoint,
PDClientHelper::getGCSafePointWithRetry, and the failing test in
gtest_new_kvstore.cpp); apply the same RAII/fixture cleanup to the other similar
block noted (lines ~1268-1305).

dbms/src/Storages/KVStore/tests/gtest_new_kvstore.cpp (1)

1281-1281: Prefer controlled staleness over wall-clock sleep.

std::this_thread::sleep_for here adds avoidable test latency and can introduce CI timing flakiness; consider a failpoint/sync-point based trigger for cache-expiry simulation.

Based on learnings: storage engine tests should rely on failpoints (and SyncPointCtl) to simulate timing/race conditions.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@dbms/src/Storages/KVStore/tests/gtest_new_kvstore.cpp` at line 1281, The test
currently uses std::this_thread::sleep_for to simulate cache expiry, which adds
latency and flakiness; replace that wall-clock sleep by driving a deterministic
test synchronisation: remove std::this_thread::sleep_for and instead trigger the
cache-expiry path via a failpoint or SyncPointCtl (e.g., enable/trigger a named
failpoint or SyncPoint and wait for its notification) so the test explicitly
advances the component under test (cache invalidation/expiry) and blocks on the
sync-point rather than sleeping; look for occurrences of
std::this_thread::sleep_for in gtest_new_kvstore.cpp and wire the test to use
the existing FailPoint/SyncPointCtl APIs to simulate the timing event.